Skip to content

Add link to coaching notes editor #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 12, 2025
Merged

Conversation

zgavin1
Copy link
Contributor

@zgavin1 zgavin1 commented Apr 3, 2025

Description

Editor feature for adding/removing links.

A user may now in the coaching notes editor incorporate hyperlink(s) into the editor content.

The toolbar button should open a dialog box with a field for the url.

Large part of this logic borrowed from TipTap's example

Link text should be highlighted a standard blue.

Addresses: https://docs.google.com/document/d/10gTCL0uNu1VHEFQl_PzPmnn07QOcDw89ta5C-UVh9vQ/edit?tab=t.0#heading=h.r4v5w0aeykos

Changes

  • Install and add link extension to extensions groups
  • Add toolbar button for link to toolbar
  • Add link dialog component
  • Logic for inserting/removing link

Screenshots / Videos Showing UI Changes (if applicable)

Screenshot 2025-04-03 at 11 37 11 AM Screenshot 2025-04-03 at 11 37 08 AM

Testing Strategy

  • Navigate to a coaching session
  • Click link button in toolbar
  • Insert a link
  • Insert another link
  • Remove a link
  • Update an existing link

Concerns

@jhodapp We may want to review some specifics with how you want this to work - validating links, disallowing some domains, etc. Will leave this in draft until we chat about that

@zgavin1 zgavin1 requested a review from jhodapp April 3, 2025 16:42
@jhodapp jhodapp added enhancement Improves existing functionality or feature feature work Specifically implementing a new feature labels Apr 3, 2025
@jhodapp jhodapp added this to the 1.0-beta1 milestone Apr 3, 2025
Copy link
Member

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great Zach, thank you for this. A few suggestions inline.

How difficult would it be to allow for updating an existing link and then following the link if one presses ?

: new URL(`https://${url}`);

// only auto-link if the domain is not in the disallowed list
const disallowedDomains = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zgavin1 I think a good central place for this list right now is to place this in site.config.ts under siteConfig { tiptap { links { } } }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also just comment this out.. Do we even want these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, it's probably better to have it than not. No immediate use but I'm guessing it's a pattern for TipTap for a good reason.

<DialogHeader>
<DialogTitle>Insert Link</DialogTitle>
<DialogDescription>
Insert a url for a link in your document.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Insert a url for a link in your document.
Insert URL for your link.

Comment on lines 147 to 150
a {
color: #0000EE;
text-decoration: underline;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed?

onClick={() => setIsLinkDialogOpen(true)}
isActive={editor.isActive("link")}
icon={<Link className="h-4 w-4" />}
title="Link"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zgavin1 Can you dynamically have this text change like so:

  • "Insert Link (Ctrl+K)" for when inserting a link
  • "Update Link (Ctrl+K)" when one has highlighted a link?

@jhodapp jhodapp moved this from Review to 🏗 In progress in Refactor Coaching Platform Apr 3, 2025
@zgavin1 zgavin1 requested a review from jhodapp April 8, 2025 01:22
@zgavin1 zgavin1 marked this pull request as ready for review April 8, 2025 01:22
@jhodapp jhodapp moved this from 🏗 In progress to Review in Refactor Coaching Platform Apr 8, 2025
jhodapp
jhodapp previously requested changes Apr 8, 2025
Copy link
Member

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions inline, looking great Zach.

Right click to open link doesn't work for me, but perhaps you can learn from this solution to give SHIFT + left click another try.

Comment on lines 122 to 123
? "Update link (Ctrl + k)"
: "Insert link (Ctrl + k)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
? "Update link (Ctrl + k)"
: "Insert link (Ctrl + k)"
? "Update Link (Ctrl + k)"
: "Insert Link (Ctrl + k)"

This makes it consistent with the other buttons' tooltip capitalization.

// Add keyboard shortcut handler
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if ((e.metaKey || e.ctrlKey) && e.key === "k") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't working today because of our command menu at the top, it listens for CMD + k as well which is in conflict. We do want to hide/remove that search for the beta anyway, so if you do that, this should work.

I changed the key combination to listen for 'l' and then CTRL + l worked for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, had to test this myself because it was definitely working for me when I committed it.

When I'm on a coaching notes page AND focus is in the editor, it works exactly as I'd expect it to (link entry dialog opens). But if focus is NOT in the editor, it behaves incorrectly.: it opens the link entry dialog AND the keyboard shortcut dialog. To me that means that the command menu is working properly (i.e. not listening for cmd + k when focus is in the editor. I could just do the reverse here, ensure the link shortcut on works when editor is focused.

@jhodapp or, were you seeing a totally different behavior?

Comment on lines 39 to 65
// Placeholder for future disallowed domains if we want to add any
// const disallowedProtocols = ["ftp", "file", "mailto"];
// const protocol = parsedUrl.protocol.replace(":", "");

// if (disallowedProtocols.includes(protocol)) {
// return false;
// }

// // only allow protocols specified in ctx.protocols
// const allowedProtocols = ctx.protocols.map((p) =>
// typeof p === "string" ? p : p.scheme
// );

// if (!allowedProtocols.includes(protocol)) {
// return false;
// }

// Placeholder for future disallowed domains if we want to add any
// const disallowedDomains = [
// "example-phishing.com",
// "malicious-site.net",
// ];
// const domain = parsedUrl.hostname;

// if (disallowedDomains.includes(domain)) {
// return false;
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed? You have implemented some or all of this already from what I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can definitely remove it. It's not the same as what I've implemented, its more like custom validation that we could absolutely hold off on implementing until its really needed.

@zgavin1 zgavin1 force-pushed the add-link-to-coaching-notes-editor branch from 4bdc197 to 4f9b6b5 Compare April 12, 2025 15:12
@zgavin1 zgavin1 requested a review from jhodapp April 12, 2025 17:01
Copy link
Collaborator

@calebbourg calebbourg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!

@zgavin1 zgavin1 dismissed jhodapp’s stale review April 12, 2025 22:19

to pass gh action

@zgavin1 zgavin1 merged commit 86845e1 into main Apr 12, 2025
5 checks passed
@zgavin1 zgavin1 deleted the add-link-to-coaching-notes-editor branch April 12, 2025 22:47
@github-project-automation github-project-automation bot moved this from Review to ✅ Done in Refactor Coaching Platform Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves existing functionality or feature feature work Specifically implementing a new feature
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants